kvserver: abstract away system config span usage in kv #69172

irfansharif · 2021-08-19T19:52:00Z

Part of #67679. We'll hide config.SystemConfig behind the
spanconfig.StoreReader interface, and use that instead in the various
queues that need access to the system config span. In future PRs we'll
introduce a data structure that maintains a mapping between spans and
configs that implements this same interface, except it will be powered
by a view over system.span_configurations that was introduced in
#69047.

When we do make that switch, i.e. have KV consult this new thing for
splits, merges, GC and replication, instead of the gossip backed system
config span, ideally it'd be as easy as swapping the source. This PR
helps pave the way for just that.

In #66348 we described how zonepb.ZoneConfigs going forward were going
to be an exclusively SQL-level construct. Consequently we purge[*] all
usages of it in KV, storing on each replica a roachpb.SpanConfig
instead.

[*]: The only remaining use is what powers our replication reports,
which does not extend well to multi-tenancy in its current form and
needs replacing.

Release note: None

Release justification: low risk, high benefit changes to existing functionality

cockroach-teamcity · 2021-08-19T19:52:10Z

This change is

irfansharif · 2021-08-19T21:42:39Z

Actually the last commit is reviewable as is.

ajwerner

Most of this change feels mechanical. When you can, it's nice to separate out mechanical changes from implementation changes to help the reviewer.

I don't think this is a particularly controversial change. I'd like Nathan to take a pass as it's the KV team which will end up owning this stuff.

pkg/kv/kvserver/store.go

irfansharif · 2021-08-24T14:47:15Z

Woops, the shadowConfReader was not meant to be in this commit (it wasn't wired up to anything anyway -- we'll use it in a later PR when we have another implementation of spanconfig.StoreReader). @nvanbenschoten, mind taking a look at the last commit when you get the chance? It's entirely mechanical.

irfansharif · 2021-08-26T14:12:07Z

(gentle bump, @nvanbenschoten)

nvanbenschoten

Reviewed 59 of 66 files at r7, 77 of 77 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @irfansharif)

pkg/kv/kvserver/allocator_scorer_test.go, line 620 at r8 (raw file):

				{
					Constraints: []zonepb.Constraint{
						{Value: "a", Type: zonepb.Constraint_DEPRECATED_POSITIVE},

This raised a red flag for me and I now see that Constraint_DEPRECATED_POSITIVE is not handled in ZoneConfig.toSpanConfig. Should it be? Do we have a guarantee that we will never see an existing zone with a Constraint_DEPRECATED_POSITIVE constraint type?

pkg/kv/kvserver/merge_queue.go, line 389 at r8 (raw file):

	}
	if testingAggressiveConsistencyChecks {
		if _, err := mq.store.consistencyQueue.process(ctx, lhsRepl, nil); err != nil {

Are we intentionally passing nil here?

pkg/kv/kvserver/replicate_queue.go, line 322 at r8 (raw file):

			if testingAggressiveConsistencyChecks {
				if _, err := rq.store.consistencyQueue.process(ctx, repl, nil); err != nil {

Same question here. Is this intentional?

pkg/kv/kvserver/store.go, line 2982 at r8 (raw file):

}

// TestingDefaultSpanConfig // XXX:

Were you planning to remove/rewrite this?

pkg/roachpb/span_config.proto, line 92 at r8 (raw file):

  // GCTTL is the number of seconds overwritten values will be retained before
  // garbage collection. A value <= 0 means older versions are never GC-ed.
  int32 gc_ttl = 3 [(gogoproto.customname) = "GCTTL"];

Out of curiosity, why did we decide to inline this instead of keeping it split out in a separate policy struct? It feel like we may have lost some future-proofing with that decision, as extensions to the GC policy along multiple dimensions (e.g. retain all deletion tombstones for incremental backup, but eagerly collect duplicate values) will now be more invasive. I wonder how @ajwerner feels about this.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @irfansharif, and @nvanbenschoten)

pkg/roachpb/span_config.proto, line 92 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Out of curiosity, why did we decide to inline this instead of keeping it split out in a separate policy struct? It feel like we may have lost some future-proofing with that decision, as extensions to the GC policy along multiple dimensions (e.g. retain all deletion tombstones for incremental backup, but eagerly collect duplicate values) will now be more invasive. I wonder how @ajwerner feels about this.

In some ways this feels like the most invasive part of this entire change. I also did not see the motivation. I let it go because I didn't see any strong reason to oppose it. I think your point about policy changes in the future is sound.

irfansharif

TFTRs! Will merge on green.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @nvanbenschoten)

pkg/kv/kvserver/allocator_scorer_test.go, line 620 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This raised a red flag for me and I now see that Constraint_DEPRECATED_POSITIVE is not handled in ZoneConfig.toSpanConfig. Should it be? Do we have a guarantee that we will never see an existing zone with a Constraint_DEPRECATED_POSITIVE constraint type?

We're guaranteed, yes. We validate against this when users set zone configs. The only remaining uses of this were in tests.

cockroach/pkg/config/zonepb/zone.go

Lines 368 to 371 in b296bac

    
           if constraint.Type == Constraint_DEPRECATED_POSITIVE { 
        
           	return fmt.Errorf("constraints must either be required (prefixed with a '+') or " + 
        
           		"prohibited (prefixed with a '-')") 
        
           }

Even looking back to the change that deprecated it, it seems they were never officially supported or documented.
73466fd

I suppose it's possible for some on-prem customer running v1.x to have this lying around, and an upgrade to 21.2 would break it cause we never blocked the upgrade with this validation. Do we care about that at all?

pkg/kv/kvserver/merge_queue.go, line 389 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Are we intentionally passing nil here?

It was, we passed in nil elsewhere for queues that don't depend on the system config span, and these queues don't either, but meh I'll pass in the available conf reader.

cockroach/pkg/kv/kvserver/store.go

Line 2879 in 56afa9f

processed, err := s.replicaGCQueue.process(ctx, repl, nil)

pkg/kv/kvserver/replicate_queue.go, line 322 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Same question here. Is this intentional?

See above.

pkg/kv/kvserver/store.go, line 2982 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Were you planning to remove/rewrite this?

No, this will be here for now, but I did forget to add a proper comment. Done.

pkg/roachpb/span_config.proto, line 92 at r8 (raw file):

Previously, ajwerner wrote…

In some ways this feels like the most invasive part of this entire change. I also did not see the motivation. I let it go because I didn't see any strong reason to oppose it. I think your point about policy changes in the future is sound.

Hm, I was inclined to simplify it given the struct in its original form was introduced with the single field back in 2014 and hadn't seen any change since. But I don't feel strongly about it either way -- changed to a similar looking policy struct.

irfansharif · 2021-08-27T20:02:54Z

bors r+

irfansharif · 2021-08-27T21:10:49Z

bors r-

craig · 2021-08-27T21:10:50Z

Canceled.

Part of cockroachdb#67679. We'll hide `config.SystemConfig` behind the `spanconfig.StoreReader` interface, and use that instead in the various queues that need access to the system config span. In future PRs we'll introduce a data structure that maintains a mapping between spans and configs that implements this same interface, except it will be powered by a view over `system.span_configurations` that was introduced in \cockroachdb#69047. When we do make that switch, i.e. have KV consult this new thing for splits, merges, GC and replication, instead of the gossip backed system config span, ideally it'd be as easy as swapping the source. This PR helps pave the way for just that. In cockroachdb#66348 we described how `zonepb.ZoneConfigs` going forward were going to be an exclusively SQL-level construct. Consequently we purge[*] all usages of it in KV, storing on each replica a `roachpb.SpanConfig` instead. [*]: The only remaining use is what powers our replication reports, which does not extend well to multi-tenancy in its current form and needs replacing. Release note: None Release justification: low risk, high benefit changes to existing functionality

irfansharif · 2021-08-30T13:05:10Z

bors r+

69172: kvserver: abstract away system config span usage in kv r=irfansharif a=irfansharif Part of #67679. We'll hide config.SystemConfig behind the spanconfig.StoreReader interface, and use that instead in the various queues that need access to the system config span. In future PRs we'll introduce a data structure that maintains a mapping between spans and configs that implements this same interface. This will be powered by a view of `system.span_configurations`, following the ideas described in \#66348. When we do make that switch, i.e. have KV consult the new thing for splits, merges, GC and replication, instead of the gossip backed system config span, ideally it'd be as easy as swapping the source. This PR helps pave the way for just that. In \#66348 we described how zonepb.ZoneConfigs going forward were going to be an exclusively SQL-level construct. Consequently we purge[*] all usages of it in KV, storing on each replica a roachpb.SpanConfig instead. [*]: The only remaining use is what powers our replication reports, which does not extend well to multi-tenancy and needs replacing. Release note: None (First two commits are from #69047; ignore here) Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>

nvanbenschoten

Reviewed 8 of 10 files at r9, 2 of 2 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @nvanbenschoten)

craig · 2021-08-30T15:00:37Z

Build failed:

GitHub CI (Cockroach)

irfansharif · 2021-08-30T15:09:38Z

bors r+

craig · 2021-08-30T16:38:46Z

Build succeeded:

GitHub CI (Cockroach)

In cockroachdb#69172 we introduced a spanconfig.StoreReader interface to abstract away the gossiped system config span. We motivated that PR by teasing a future implementation of the same interface, an in-memory data structure to maintain a mapping between between spans and configs (powered through a view over system.span_configurations introduced in \cockroachdb#69047). This PR introduces just that. Intended (future) usages: - cockroachdb#69614 introduces the KVWatcher interface, listening in on system.span_configurations. The updates generated by it will be used to populate per-store instantiations of this data structure, with an eye towards providing a "drop-in" replacement of the gossiped system config span (conveniently implementing the sibling spanconfig.StoreReader interface). - cockroachdb#69661 introduces the SQLWatcher interface, listening in on changes to system.{descriptor,zones} and generating denormalized span config updates for every descriptor/zone config change. These updates will need to be diffed against a spanconfig.StoreWriter populated with the existing contents of KVAccessor to generate the "targeted" diffs KVAccessor expects. Release note: None

70287: spanconfig: introduce spanconfig.StoreWriter (and its impl) r=irfansharif a=irfansharif In #69172 we introduced a spanconfig.StoreReader interface to abstract away the gossiped system config span. We motivated that PR by teasing a future implementation of the same interface, an in-memory data structure to maintain a mapping between between spans and configs (powered through a view over system.span_configurations introduced in [#69047](69047)). This PR introduces just that. Intended (future) usages: - [#69614](69614) introduces the KVWatcher interface, listening in on system.span_configurations. The updates generated by it will be used to populate per-store instantiations of this data structure, with an eye towards providing a "drop-in" replacement of the gossiped system config span (conveniently implementing the sibling spanconfig.StoreReader interface). - [#69661](69661) introduces the SQLWatcher interface, listening in on changes to system.{descriptor,zones} and generating denormalized span config updates for every descriptor/zone config change. These updates will need to be diffed against a spanconfig.StoreWriter populated with the existing contents of KVAccessor to generate the "targeted" diffs KVAccessor expects. Release note: None Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>

Fixes cockroachdb#75109. There are two ways to read the configuration applied over a given replica: (a) the copy embedded in each replica struct (b) spanconfig.StoreReader, hanging off the store struct The interface in (b) is implemented by both the span configs infrastructure and the legacy system config span it's designed to replace; it's typically used by KV queues (see cockroachdb#69172). When switching between subsystems in KV (controlled by spanconfig.store.enabled), for we transparently switch the source for (b). We also use then use the right source to refresh (a) for every replica. Incremental updates thereafter mutate (a) directly. As you'd expect, we need to take special care that only one subsystem is writing to (a) at a given point in time, a guarantee that was not upheld before this commit. This bug manifested after we enabled span configs by default (see cockroachdb#73876), and likely affected many tests. To avoid the system config span from clobbering (a) when span configs are enabled, we just need to check what spanconfig.store.enabled holds at the right spot. To repro: # Reliably fails with 1-2m on my GCE worker before this patch, # doesn't after. dev test pkg/kv/kvserver \ -f TestBackpressureNotAppliedWhenReducingRangeSize \ -v --show-logs --timeout 15m --stress Release note: None

74765: roachpb: introduce the concept of `SystemSpanConfig` and related protos r=arulajmani a=arulajmani This patch is motivated by the desire to let the host tenant lay protected timestamps on one or all secondary tenants' keyspace. It also provides a mechanism to allow secondary tenants to lay protected timestamps on their entire keyspace without updating every span configuration. We introduce the concept of `SystemSpanConfig` and `SystemSpanConfigTarget` to enable this. We tie these together using a `SystemSpanConfigEntry`. A `SystemSpanConfig` is a system installed configuration that can apply to multiple spans. It only contains protected timestamp information. A `SystemSpanConfigTarget` is used to specify the spans a `SystemSpanConfig` applies over. It can be used to target the entire (logical) cluster or a particular secondary tenant. We will ensure that only the host tenant can target secondary tenants in a future PR that actually persists `SystemSpanConfigs`. We will persist `SystemSpanConfigs` in `system.span_configurations` in a future patch. The `SystemSpanConfigTarget` will be encoded into special reserved keys when we do so. This change introduces the notion of a hierarchy to span configurations. The configuration that applies to a span will now bee the `SpanConfig` stored in `system.span_configurations` combined with all the `SystemSpanConfigs` that apply to the span. This can be at most 4 levels deep -- for a secondary tenant's range, the secondary tenant can install a `SystemSpanConfig` that applies to all its ranges, the host tenant can install a `SystemSpanConfig` that applies to all ranges of the secondary tenant, and the host tenant can install a `SystemSpanConfig` that applies to all ranges. These protos form the data model which will later be used to enable protected timestamp support for secondary tenants using the span config infrastructure. It will be used by the various components such as the `SQLTranslator`, `KVAccessor`, `Reconciler` etc. Release note: None 75233: kvserver: avoid clobbering replica conf r=irfansharif a=irfansharif Fixes #75109. There are two ways to read the configuration applied over a given replica: (a) the copy embedded in each replica struct (b) spanconfig.StoreReader, hanging off the store struct The interface in (b) is implemented by both the span configs infrastructure and the legacy system config span it's designed to replace; it's typically used by KV queues (see #69172). When switching between subsystems in KV (controlled by spanconfig.store.enabled), for we transparently switch the source for (b). We also use then use the right source to refresh (a) for every replica. Incremental updates thereafter mutate (a) directly. As you'd expect, we need to take special care that only one subsystem is writing to (a) at a given point in time, a guarantee that was not upheld before this commit. This bug manifested after we enabled span configs by default (see #73876), and likely affected many tests. To avoid the system config span from clobbering (a) when span configs are enabled, we just need to check what spanconfig.store.enabled holds at the right spot. To repro: # Reliably fails with 1-2m on my GCE worker before this patch, # doesn't after. dev test pkg/kv/kvserver \ -f TestBackpressureNotAppliedWhenReducingRangeSize \ -v --show-logs --timeout 15m --stress Release note: None 75280: sql: deprecate TableDescriptor.GCMutations r=postamar,ajwerner a=stevendanna This appears unused. While the schema changer adds entries that the gc job subsequently removes, the only other code that made use of this field (outside of tests) was FindIndexByID. FindIndexByID appears to use it to return a special error that no one looks for. Release note: None Co-authored-by: arulajmani <arulajmani@gmail.com> Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com> Co-authored-by: Steven Danna <danna@cockroachlabs.com>

irfansharif mentioned this pull request Aug 19, 2021

*: rework zone configurations to support tenants #67679

Closed

21 tasks

irfansharif marked this pull request as ready for review August 19, 2021 21:42

irfansharif requested a review from a team August 19, 2021 21:42

irfansharif requested review from a team as code owners August 19, 2021 21:42

irfansharif requested a review from a team August 19, 2021 21:42

irfansharif requested review from a team as code owners August 19, 2021 21:42

irfansharif requested review from dt and erikgrinaker and removed request for a team August 19, 2021 21:42

irfansharif requested review from nvanbenschoten, ajwerner and arulajmani and removed request for dt and erikgrinaker August 19, 2021 21:42

irfansharif force-pushed the 210818.abstract-away-syscfg branch from 861bcef to cfcc110 Compare August 24, 2021 02:56

ajwerner reviewed Aug 24, 2021

View reviewed changes

pkg/kv/kvserver/store.go Outdated Show resolved Hide resolved

pkg/kv/kvserver/store.go Outdated Show resolved Hide resolved

pkg/kv/kvserver/store.go Outdated Show resolved Hide resolved

pkg/kv/kvserver/store.go Outdated Show resolved Hide resolved

irfansharif force-pushed the 210818.abstract-away-syscfg branch from cfcc110 to c458484 Compare August 24, 2021 14:45

irfansharif force-pushed the 210818.abstract-away-syscfg branch 3 times, most recently from c6ef90f to b296bac Compare August 26, 2021 14:11

nvanbenschoten approved these changes Aug 26, 2021

View reviewed changes

ajwerner reviewed Aug 27, 2021

View reviewed changes

irfansharif commented Aug 27, 2021

View reviewed changes

irfansharif force-pushed the 210818.abstract-away-syscfg branch 2 times, most recently from 3eb95e7 to 87a99c4 Compare August 27, 2021 20:02

irfansharif force-pushed the 210818.abstract-away-syscfg branch from 87a99c4 to 2e00964 Compare August 27, 2021 22:06

nvanbenschoten approved these changes Aug 30, 2021

View reviewed changes

craig bot merged commit a0bfc85 into cockroachdb:master Aug 30, 2021

irfansharif deleted the 210818.abstract-away-syscfg branch August 30, 2021 17:10

irfansharif mentioned this pull request Sep 15, 2021

spanconfig: introduce spanconfig.StoreWriter (and its impl) #70287

Merged

irfansharif mentioned this pull request Jan 20, 2022

kvserver: avoid clobbering replica conf #75233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: abstract away system config span usage in kv #69172

kvserver: abstract away system config span usage in kv #69172

irfansharif commented Aug 19, 2021 •

edited

Loading

cockroach-teamcity commented Aug 19, 2021

irfansharif commented Aug 19, 2021

ajwerner left a comment

irfansharif commented Aug 24, 2021

irfansharif commented Aug 26, 2021

nvanbenschoten left a comment

ajwerner left a comment

irfansharif left a comment

irfansharif commented Aug 27, 2021

irfansharif commented Aug 27, 2021

craig bot commented Aug 27, 2021

irfansharif commented Aug 30, 2021

nvanbenschoten left a comment

craig bot commented Aug 30, 2021

irfansharif commented Aug 30, 2021

craig bot commented Aug 30, 2021

	if constraint.Type == Constraint_DEPRECATED_POSITIVE {
	return fmt.Errorf("constraints must either be required (prefixed with a '+') or " +
	"prohibited (prefixed with a '-')")
	}

kvserver: abstract away system config span usage in kv #69172

kvserver: abstract away system config span usage in kv #69172

Conversation

irfansharif commented Aug 19, 2021 • edited Loading

cockroach-teamcity commented Aug 19, 2021

irfansharif commented Aug 19, 2021

ajwerner left a comment

Choose a reason for hiding this comment

irfansharif commented Aug 24, 2021

irfansharif commented Aug 26, 2021

nvanbenschoten left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

irfansharif commented Aug 27, 2021

irfansharif commented Aug 27, 2021

craig bot commented Aug 27, 2021

irfansharif commented Aug 30, 2021

nvanbenschoten left a comment

Choose a reason for hiding this comment

craig bot commented Aug 30, 2021

irfansharif commented Aug 30, 2021

craig bot commented Aug 30, 2021

irfansharif commented Aug 19, 2021 •

edited

Loading